-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Add SVCB record #110
base: master
Are you sure you want to change the base?
WIP: Add SVCB record #110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you very much for your contribution. I've added some feedback. Could you also squash all commits into one? That would be much appreciated.
/** | ||
* SVCB Record Type (Service binding) | ||
* | ||
* https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link, and all other links in javadoc in this file, should ideally be using @see
. For example in this case
* @see <a href="https://tools.ietf.org/html/draft-ietf-dnsop-svcb-https-01">draft-ietf-dnsop-svcb-https-01: Service binding and parameter specification via the DNS (DNS SVCB and HTTPS RRs)</a>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm going to add it.
|
||
// The first group is the key. They key can only be a-z, 0-9 or "-" | ||
// The second group is the value. It can be a lot of things (see https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01#section-2.1.1) | ||
// except for DQUOTE (hence it can be excluded from the regex-group) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: superfluous whitespace at the front
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant as an indent for the previous line (which would be too long as a single line)
|
||
/** | ||
* The priority indicates the SvcRecordType. | ||
* https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01#section-2.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use @see
/** | ||
* SvcFieldValue | ||
* A set of key=value pairs. | ||
* https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01#section-2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use @see
} | ||
|
||
/** | ||
* Parses pairs according to format from https://tools.ietf.org/html/draft-ietf-dnsop-svcb-httpssvc-01#section-2.1.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use @see
.
while(matcher.find()) { | ||
values.put(matcher.group(1), matcher.group(2)); | ||
} | ||
return values; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The map should here be made immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
} | ||
|
||
private String createValuesString() { | ||
StringBuilder builder = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be some escaping for commas in this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like I have to update this regardless. There are two RFCs floating around for this RR and I based this on the older one. The one I linked in the PR description is the correct one.
Right now the wireformat is not valid.
I updated the PR with the comments. One thing that I think is still wrong is how the values of the key-value pairs are decoded (key decoding is fine, it's a plain UShort). According to the RFC the value is an octet String (which is why I currently simply decode with UTF-8), but the Appendix has some encoding for the value. I can't make any sense of it, but maybe you can? |
I'm going to close this PR. I don't have enough knowledge about the encoding and wireformat to make this work. I published my last changes (which don't work right now). SVCB support would be nice though, it'd be awesome if you could somehow finish this, if you've got time. |
I think it is best to mark this PR as WIP and leave it open then. |
String blobAsString = new String(blob, StandardCharsets.UTF_8); | ||
Matcher matcher = valuesPattern.matcher(blobAsString); | ||
while(matcher.find()) { | ||
values.put(matcher.group(1), matcher.group(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be some unescaping for commas in this method?
SVCB is a new record type currently in draft: https://tools.ietf.org/html/draft-ietf-dnsop-svcb-https-01
Support for it has already been added to Safari in iOS 14 (where it is used by default instead of A/AAAA), Cloudflare also added support for it just recently. Mozilla announced to implement it soon as well.
This PR adds the TYPE (RR type 64 according to the RFC) and the Data class for it.